-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify simple template #74
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments rationalizing the decisions here.
I wonder whether this should be simplified further, to a point where it doesn't have the counter code. This way it would make a useful first step on the counter tutorial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super happy with this, I think having separate modules would work better.
This simplifies the template a lot I think.
This template still leaves some beginner questions unanswered and might leave the user confused:
|
In the simple template, you don't. Put another way, the simple template should be a good template for basing the tutorials on. I'd actually like to remove the counter code and make it just a hello world instead. That way our hello world quick start tutorial becomes:
We do document this, in the second counter tutorial, in https://ratatui.rs/recipes/apps/color-eyre/, in https://github.com/ratatui/ratatui/blob/main/examples/README.md#design-choices (documenting why we use this in all the examples). Additionally hovering over the color_eyre line shows a decent message that answers the question. Including color-eyre in the default template is an opinionated default. It's there because the value it adds when something fails outweighs any downsides. I'll move everything under simple/template and add a readme to this to help.
I have a strong (evidence based) opinion against pre-emptively organizing a project into horizontal layers. Having worked on many projects small and large, horizontal segmentation leads to poorer long term maintainability for a variety of reasons. Doing this would be pre-emptive. Put another way, if I was using the simple template personally, having the extra file would be an annoyance that I'd always want to revert. Reverting would be annoying. Refactoring in the other direction is fairly simple though - highlight code, extract to module, done.
|
Remove cargo.lock as this will get out of sync with any dependency bump
Moved the template under simple/template so that we can have a readme for the template and a generated readme for the template user. Added documentation for color-eyre, fixed up the readme outline of the files, added github workflows and dependabot settings. Removed cargo.lock as it will get out of sync pretty quickly and running cargo on the generated source will effectively update to the latest immediately. |
I'm not sure adding GitHub workflows to the templates. It might increase complexity, especially for new users who would need to understand what those 100 lines of opinionated YAML is doing, how to use GitHub Actions, etc. Additionally, it's not directly related to the context of TUIs as well. Personally, I’d find it annoying and would likely delete the |
This is a template, stored on github to create an application. Generating github workflows to ensure that the project builds well is a useful thing to do. Anyone using GitHub should have a basic understanding of github workflows, which are well documented. The "opinionated" parts of these are fairly insignificant and the sort that I wonder why you'd delete. They just check formatting, check clippy lints, check docs, and run any tests. These are basic hygiene things for any rust project. Given that deleting these is a matter of deleting a single file, and that this generally makes any project better, I'm for leaving these in. |
I made a couple of minor comments but it looks good to me overall. |
It just felt a bit too much for a simple project creation, that's all. But yes, I agree with your point that the user can just delete them, so it shouldn't be a big deal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slapping an approval here not to block this further, might come back to it later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the counter behavior and the events module entirely as it was not a "simple" implementation and It's not really necessary for simple apps at all to worry about a tick event. This makes this a good basis for the simple tutorials.
simple-generated/src/event.rs
Outdated
pub struct EventHandler { | ||
/// Event sender channel. | ||
sender: mpsc::Sender<Event>, | ||
pub struct EventSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike EventHandler here as the application code needs to handle events somewhere. Event handler would not be that place. The missing info in the prompt is that the event handler both handles the event and sends the tick event. I'm inclined to perhaps actually simplify this further and remove events as used here altogether.
simple-generated/src/event.rs
Outdated
pub struct EventHandler { | ||
/// Event sender channel. | ||
sender: mpsc::Sender<Event>, | ||
pub struct EventSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the entire module in 6a76940
Let's KISS on this template, and move the more opinionated ideas about event handling, threads, etc. to another more advanced template that documents them well. |
Merging this to be able to rewrite the getting started docs. |
Sounds good, thanks for your work on this 👍🏼 |
This will flow through to the async template too once the various issues here are resolved.